Refactor: move per-realm advisory lock into DBAdapter.withWriteLock#4839
Merged
Conversation
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the per-realm Postgres advisory write-lock from a realm-server-local helper into the shared DBAdapter interface, implemented by PgAdapter (real lock + pinned transaction) and a SQLite passthrough. This is intended to keep Node-only plumbing out of browser bundles and make the lock callable from shared runtime-common code.
Changes:
- Add
DBAdapter.withWriteLock(realmUrl, fn)topackages/runtime-common/db.tsand update realm-server callers to use it. - Implement
withWriteLock(and exporthashRealmUrlForAdvisoryLock) inpackages/postgres/pg-adapter.ts; add SQLite passthrough implementation. - Remove
packages/realm-server/lib/realm-advisory-locks.tsand update tests + local DBAdapter stubs accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/db.ts | Extends DBAdapter interface with withWriteLock callback API. |
| packages/postgres/pg-adapter.ts | Implements advisory-lock + transaction-scoped lock acquisition; exports canonical realm-url hash helper. |
| packages/host/app/lib/sqlite-adapter.ts | Adds SQLite passthrough withWriteLock implementation. |
| packages/realm-server/server.ts | Switches realm creation to use dbAdapter.withWriteLock. |
| packages/realm-server/handlers/handle-publish-realm.ts | Switches publish flow to dbAdapter.withWriteLock (but currently drops prior cache invalidation step; see review comment). |
| packages/realm-server/handlers/handle-unpublish-realm.ts | Switches unpublish cleanup to dbAdapter.withWriteLock. |
| packages/realm-server/handlers/handle-delete-realm.ts | Switches delete cleanup to dbAdapter.withWriteLock. |
| packages/realm-server/lib/realm-advisory-locks.ts | Deletes the old helper module. |
| packages/realm-server/tests/realm-advisory-locks-test.ts | Updates tests to call PgAdapter.withWriteLock and import hash helper from @cardstack/postgres. |
| packages/realm-server/tests/realm-cleanup-transaction-test.ts | Updates regression test to call dbAdapter.withWriteLock. |
| packages/realm-server/tests/indexing-event-sink-test.ts | Adds withWriteLock to inline adapter stubs. |
| packages/realm-server/tests/prerender-proxy-test.ts | Adds withWriteLock to inline adapter stub. |
| packages/realm-server/tests/screenshot-card-test.ts | Adds withWriteLock to inline adapter stub. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Preview deploymentsHost Test Results 1 files 1 suites 1h 59m 20s ⏱️ Results for commit 87eb4d8. Realm Server Test Results 1 files ± 0 1 suites ±0 8m 3s ⏱️ + 1m 25s Results for commit 87eb4d8. ± Comparison against earlier commit 450e521. |
`withRealmWriteLock` previously lived in `packages/realm-server/lib/` and was only reachable from realm-server code. Moving the lock primitive onto the `DBAdapter` interface as a `withWriteLock` method keeps the implementation (and its `crypto` + `pg_advisory_xact_lock` plumbing) in @cardstack/postgres while letting any DBAdapter consumer call it through the existing interface — including code in runtime-common that ships to the browser, which previously couldn't import the helper without leaking Node-only crypto into the browser bundle. - packages/runtime-common/db.ts: add required `withWriteLock` method to the `DBAdapter` interface. - packages/postgres/pg-adapter.ts: implement on PgAdapter. Move `hashRealmUrlForAdvisoryLock` here as the canonical hash helper. - packages/host/app/lib/sqlite-adapter.ts: no-op passthrough — SQLite has no cross-connection concurrency to coordinate. - packages/realm-server: callers (`server.ts`, publish/unpublish/delete handlers, tests) call `dbAdapter.withWriteLock(url, fn)` directly; the old `realm-advisory-locks.ts` is deleted. Tests that build inline DBAdapter stubs gain a passthrough `withWriteLock`. No behavior change. Same advisory lock semantics, same callers, same transactional cleanup contract for CS-10898 (the pinned txQuerier is unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lukemelia
force-pushed
the
withwritelock-refactor
branch
from
450e521 to
87eb4d8
Compare
lukemelia
added a commit
that referenced
this pull request
May 14, 2026
The advisory lock from CS-10891 wrapped only the realm-lifecycle handlers (create / publish / unpublish / delete realm). The data-plane mutation routes that normal client traffic actually hits (POST/PATCH/DELETE <realm>/<path>, POST <realm>/_atomic) didn't take it — fine on a single replica because the Node event loop is the implicit serializer, but a real lost-update race the moment a second replica appears behind an LB. This PR threads the per-realm `withWriteLock` (newly exposed on `DBAdapter` by the parent refactor) through the data-plane mutations in `packages/runtime-common/realm.ts`: - `write` / `writeMany` / `delete` / `deleteAll` wrap their inner work in the lock. Inner methods are renamed `_batchWriteUnlocked` / `_deleteUnlocked` / `_deleteAllUnlocked` so callers that need the lock around a wider critical section can re-use the inner without re-entering the lock (which would block on a different pinned pool connection). - `handleAtomicOperations` takes the lock at the handler boundary so the `add`/`update` precheck is in the same critical section as the writeMany — fixes the `/_atomic` TOCTOU spelled out in the ticket. - `patchCardInstance` takes the lock around the indexEntry read + merge + write, so two concurrent PATCHes from different replicas can't both read the same `original` and clobber each other's merge. Lock granularity stays per-realm — concurrent writes to different realms remain fully parallel. SQLite (host) is a passthrough (no cross-connection concurrency to coordinate), so this is a no-op there. ## Regression test `packages/realm-server/tests/data-plane-write-lock-test.ts`: 1. Two concurrent PATCHes against the same card with non-overlapping attribute changes — both must persist. Without the lock the second writer would compute its merge from pre-first state and drop the first writer's field. 2. Two concurrent `/_atomic` `add` operations on the same href — must yield 201 + 409, not 201 + 201. Without the lock both pass the precheck and both write. Compatible with single-replica deploy: the lock is a no-op without contention. Lands before CS-10899 (atomicity follow-up) and the cache-invalidation tickets that assume writes are already serialized. Stacks on #4839 (DBAdapter.withWriteLock refactor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
should create realm also be using this lock? |
habdelra
approved these changes
May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/realm-server/lib/to awithWriteLockmethod on theDBAdapterinterface, with the real Postgres implementation living in@cardstack/postgres.crypto+pg_advisory_xact_lockplumbing Node-only (it never reaches the browser bundle), and unblocks the next PR (CS-11125) from calling the lock frompackages/runtime-common/realm.ts.Behavior is unchanged — same advisory lock semantics, same callers, same CS-10898 pinned-
txQueriertransactional cleanup contract.What's in
packages/runtime-common/db.ts— addswithWriteLock(realmUrl, fn(txQuerier))to theDBAdapterinterface.packages/postgres/pg-adapter.ts— implements onPgAdapterand re-exportshashRealmUrlForAdvisoryLockas the canonical hash helper.packages/host/app/lib/sqlite-adapter.ts— passthrough (no cross-connection concurrency to coordinate;txQuerieris undefined).packages/realm-server/{server.ts, handlers/handle-{publish,unpublish,delete}-realm.ts}— callers usedbAdapter.withWriteLock(...)directly.packages/realm-server/lib/realm-advisory-locks.ts— deleted.realm-advisory-locks-test,realm-cleanup-transaction-testupdated to the method form. Inline DBAdapter test stubs inindexing-event-sink-test,prerender-proxy-test,screenshot-card-testget a passthroughwithWriteLock.Test plan
pnpm test-module TEST_MODULE="realm-advisory-locks-test.ts"— 7/7 pass after the rename.pnpm test-module TEST_MODULE="realm-cleanup-transaction-test.ts"— 3/3 pass; confirms CS-10898 transactional behavior survives the method form.pnpm exec tsc -p packages/runtime-common --noEmitand-p packages/postgres,-p packages/realm-server,-p packages/host— zero new type errors vs. main.🤖 Generated with Claude Code